sdk: raise NovemException on API errors instead of a print placeholder#236
Conversation
|
@sondove I know there are concerns here. Maybe find a way to opt in to hard-failures, vs the default way of just |
|
I mean, we should definitely not raise any exceptions on 409, that is an idiomatic response in how the novem API works. The library will ALWAYS generate a |
|
Looking at this PR, i believe it's OK to go, replacing the current incomplete error handling with an exception should be fine. Those error cases have historically been deviant behavior, and an exception is a better response than a print. |
Every resource write path (vis/group/job/repo `create_*` PUT and `api_write` POST) handled 404/403/409 but, on any other non-ok response, printed the body + headers + the literal "should raise a general error" and returned — so the caller got no exception and silently no-op'd. Terje hit exactly this (novem-code/gaia#2656): `mail.bcc = [over-cap list]` returned without error while nothing was stored. The server now rejects over-cap / unresolvable writes with a 400 + `{message, rejected:[…]}`, but the SDK swallowed it. Add `raise_on_response(r)` (api_ref) — parses the server `message` and any `rejected` lines and raises the matching NovemException subclass (401/403/ 404/409 → generic NovemException otherwise) — and call it from all eight write sites. `mail.bcc = …` over the cap now raises NovemException with the cap reason and the rejected addresses.
bjornars
force-pushed
the
bsn/sdk-raise-on-api-error
branch
from
8da8a15 to
ec0f5c9
Compare
I've fixed up the 409s, and I've got some integration tests loaded up in gaia for after merge. |
Add a README "Error handling" section covering the two newest changes: - PR #236: writes now raise NovemException (or a subclass) carrying the server message instead of silently printing a placeholder. Documents the exception hierarchy (all importable from novem.exceptions) and the create PUT 409 no-op for objects that already exist. - PR #240: requests now time out instead of hanging, with the default (10s, 2min) and job.run() (30s, 30min) values and the resulting requests.exceptions.Timeout.
2 participants
Draft — implements the fail-hard option from #235 for discussion.
What
Adds
raise_on_response(r)(api_ref) — parses the servermessageand anyrejectedlines and raises the matchingNovemExceptionsubclass (401/403/404, genericNovemExceptionotherwise) — and replaces theprint("should raise a general error")placeholder in all eight write sites (vis/group/job/repo, thecreate_*PUT +api_writePOST).409 is treated as a platform-wide no-op (idempotency):
raise_on_responsereturns early on 409, so the existing per-call carve-outs inapi_createare now redundant but kept for clarity.Novem409remains available for callers that want to raise it explicitly.403s from
api_writepreviously raisedNovem403with no message; they now flow throughraise_on_responseand surface the server message.Why draft
This is the fail-hard direction from #235 — a behavioral change: writes that previously silently no-op'd on a non-2xx now raise. The open questions there should be settled before this goes ready:
raise_on_error/strictflag vs hard default (existing users)Verified
Against a live dev API,
mail.bcc = [30 addrs over the 25 cap]now raises instead of silently storing nothing:The server-side 400 that makes this observable is
novem-code/gaia#2810.Before ready
rejectedbody → raises with the reasonRefs #235 · SDK half of
novem-code/gaia#2656